Skip to content

Conversation

@dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Oct 7, 2025

New version has fixed problem with prepared statement invalidation and Driver3WorkerCQLIT needs to be fixed accordingly.

@dkropachev dkropachev force-pushed the dk/update-scylla-driver branch 2 times, most recently from a5507da to ca7dbcc Compare October 16, 2025 18:08
@mykaul
Copy link

mykaul commented Oct 16, 2025

3.11.5.9, not 3.11.5.8

@dkropachev dkropachev changed the title Update scylla driver core to 3.11.5.8 Update scylla driver core to 3.11.5.9 Oct 16, 2025
@dkropachev
Copy link
Collaborator Author

3.11.5.9, not 3.11.5.8

Yeah, it was hanging there for a long time, just updated it to 3.11.5.9

Currently it is spread over all the packages.
Let's have one place where it is defined.
@dkropachev dkropachev force-pushed the dk/update-scylla-driver branch 5 times, most recently from 8dabfd2 to 21f2404 Compare October 17, 2025 02:24
@dkropachev dkropachev requested a review from Bouncheck October 17, 2025 02:25
@dkropachev dkropachev self-assigned this Oct 17, 2025
@dkropachev dkropachev marked this pull request as ready for review October 17, 2025 02:40
@dkropachev dkropachev force-pushed the dk/update-scylla-driver branch 2 times, most recently from b1cf05a to b9b97f8 Compare October 19, 2025 21:01
@dkropachev
Copy link
Collaborator Author

@Bouncheck , WDYT should we remove these tests ? IMHO this has to be tested on the driver side and other part of this issue is handled in AlterTabletIT.java

@Bouncheck
Copy link
Collaborator

Bouncheck commented Oct 20, 2025

I see no reason to remove BaseScyllaIntegrationTest.
If all invariants in Driver3WorkerCQLIT were broken then we should have a clear understanding why it's ok now.
For broken invariants there is no need to fix their tests. I'd mark them as ignored, document and remove later on.
If there are new important invariants then those should have new tests.

@Bouncheck
Copy link
Collaborator

Bouncheck commented Oct 20, 2025

Of course if something is guaranteed not to change on the driver side then it does not necessarily need a test in cdc library.

@dkropachev
Copy link
Collaborator Author

I see no reason to remove BaseScyllaIntegrationTest. If all invariants in Driver3WorkerCQLIT were broken then we should have a clear understanding why it's ok now. For broken invariants there is no need to fix their tests. I'd mark them as ignored, document and remove later on. If there are new important invariants then those should have new tests.

It was broken because metadata was not changing because of the following logic:

                      ColumnDefinitions newMetadata = null;
                      if (rows.metadata.metadataId != null) {
                        newMetadata = rows.metadata.columns;
                        assert statement instanceof BoundStatement;
                        BoundStatement bs = (BoundStatement) statement;
                        bs.preparedStatement().getPreparedId().resultSetMetadata =
                            new PreparedId.PreparedMetadata(
                                rows.metadata.metadataId, rows.metadata.columns);
                      }
                      MultiPage.this.nextPages.offer(new NextPage(newMetadata, rows.data));

It was broken the whole time.

@Bouncheck
Copy link
Collaborator

Bouncheck commented Oct 20, 2025

I wonder if with the driver change this part below should be changed somehow or not.

Row row = rs.one();
if (schema == null) {
try {
schema = Driver3SchemaFactory.getChangeSchema(row, session.getCluster().getMetadata());
} catch (Driver3SchemaFactory.UnresolvableSchemaInconsistencyException ex) {
fut.completeExceptionally(ex);
return;
}
}
Driver3RawChange newChange = new Driver3RawChange(row, schema);

It seems like we may not be fully leveraging the "skipMetadata" improvements if we do not change it.
In this snippet the schema field is set only once per PreparedStatement. IIUC it will not be in sync with contents of the ResultSet if an ALTER operation is performed during reading

Copy link
Collaborator

@Bouncheck Bouncheck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests for old invariants should be removed, not adjusted to pass:
This invariant will not be true anymore after driver update:

   public void testPreparedStatementSameSchemaBetweenPages() throws ExecutionException, InterruptedException, TimeoutException {
        // Tests an assumption used in the implementation
        // that given a PreparedStatement, it will always
        // return the rows with the same schema - in this
        // case: when a schema change happens between
        // pages of a query.
        // Note that Driver3Session deliberately sets
        // a protocol version to V4, while these
        // tests do not. This is on purpose:
        // if Scylla in the future implements a
        // support for new protocol version, this
        // test might fail and you (are you investing
        // this failure now?) should decide how to proceed.
        // See Driver3WorkerCQL for more context.

and this too:

    public void testPreparedStatementOldSchemaAfterAlter() throws ExecutionException, InterruptedException, TimeoutException {
        // Tests an assumption used in the implementation
        // that given a PreparedStatement, it will always
        // return the rows with the same schema - in this
        // case: when a query is prepared, then a schema
        // change happens and afterwards the query is started
        // it will still return the results with the
        // schema as of the time of preparing the query.

Alternatively we could have a test that the schema is fresh between pages. Maybe one day we'll change to full protocol v5 without patches and this would guard against a mistake during transition.

@dkropachev
Copy link
Collaborator Author

I wonder if with the driver change this part below should be changed somehow or not.

Row row = rs.one();
if (schema == null) {
try {
schema = Driver3SchemaFactory.getChangeSchema(row, session.getCluster().getMetadata());
} catch (Driver3SchemaFactory.UnresolvableSchemaInconsistencyException ex) {
fut.completeExceptionally(ex);
return;
}
}
Driver3RawChange newChange = new Driver3RawChange(row, schema);

It seems like we may not be fully leveraging the "skipMetadata" improvements if we do not change it.
In this snippet the schema field is set only once per PreparedStatement. IIUC it will not be in sync with contents of the ResultSet if an ALTER operation is performed during reading

Yes exactly, that change was missing in the PR, for it is already too late, issue is fixed and metadata updates are working.

@dkropachev
Copy link
Collaborator Author

The tests for old invariants should be removed, not adjusted to pass: This invariant will not be true anymore after driver update:

   public void testPreparedStatementSameSchemaBetweenPages() throws ExecutionException, InterruptedException, TimeoutException {
        // Tests an assumption used in the implementation
        // that given a PreparedStatement, it will always
        // return the rows with the same schema - in this
        // case: when a schema change happens between
        // pages of a query.
        // Note that Driver3Session deliberately sets
        // a protocol version to V4, while these
        // tests do not. This is on purpose:
        // if Scylla in the future implements a
        // support for new protocol version, this
        // test might fail and you (are you investing
        // this failure now?) should decide how to proceed.
        // See Driver3WorkerCQL for more context.

and this too:

    public void testPreparedStatementOldSchemaAfterAlter() throws ExecutionException, InterruptedException, TimeoutException {
        // Tests an assumption used in the implementation
        // that given a PreparedStatement, it will always
        // return the rows with the same schema - in this
        // case: when a query is prepared, then a schema
        // change happens and afterwards the query is started
        // it will still return the results with the
        // schema as of the time of preparing the query.

Alternatively we could have a test that the schema is fresh between pages. Maybe one day we'll change to full protocol v5 without patches and this would guard against a mistake during transition.

I have dropped the whole suite, please take a look.

@dkropachev dkropachev requested a review from Bouncheck October 20, 2025 16:45
I would want to see all failures on all images.
No reason to fail fast.
These set of tests verify certain driver behavior.
We remove them by following reasons:
1. This behavior is not relevant anymore, we actually want driver to change
   schema in between pages.
2. There is no reason to test driver behavior in this repo, it needs to
   be tested on the driver.
@dkropachev dkropachev force-pushed the dk/update-scylla-driver branch from 8fd212b to abec814 Compare October 20, 2025 16:46
@dkropachev dkropachev merged commit 3d12af5 into scylladb:master Oct 20, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants